Skip to content

thread_storage: Fix incorrectly to detect C TLS existence#12067

Merged
edsiper merged 4 commits into
masterfrom
cosmo0920-mitigate-c-tls-fallback-incorrectly-working
Jul 11, 2026
Merged

thread_storage: Fix incorrectly to detect C TLS existence#12067
edsiper merged 4 commits into
masterfrom
cosmo0920-mitigate-c-tls-fallback-incorrectly-working

Conversation

@cosmo0920

@cosmo0920 cosmo0920 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

We incorrectly used __tls_get_addr in thread storage detection.
Instead, we just use __thread attribute in that detector and ensuring pthread thread storage to be initialized once on each of the threads.
Also, I added a test cases for confirming define/get/set of thread storage.

Closes #12065.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

Summary by CodeRabbit

  • Bug Fixes
    • Improved build-time detection for thread-local storage support to ensure correct TLS behavior on more toolchains.
    • Updated worker initialization ordering to improve reliability of thread-local storage across worker threads.
  • Tests
    • Added internal pthread-based unit tests covering concurrent TLS access and verifying TLS state after initialization and clearing.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 15fe5dfd-1d78-4b15-baff-a685960c2eaa

📥 Commits

Reviewing files that changed from the base of the PR and between 645d6ed and debb55e.

📒 Files selected for processing (2)
  • src/flb_config.c
  • tests/internal/thread_storage.c
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/internal/thread_storage.c
  • src/flb_config.c

📝 Walkthrough

Walkthrough

This PR changes the C TLS feature probe, moves flb_worker_init earlier in configuration setup, and adds internal unit tests for thread-local storage behavior during concurrent access and after initialization.

Changes

TLS Initialization Fix and Test Coverage

Layer / File(s) Summary
C TLS compile-time detection update
CMakeLists.txt
The FLB_HAVE_C_TLS probe now assigns a = 1 to a thread-local variable and returns a - 1, replacing the previous __tls_get_addr(0) call and fixed return 0.
Worker TLS init ordering in flb_config_init
src/flb_config.c
flb_worker_init(config) now runs immediately after allocating struct flb_config, and the later call under "Prepare worker interface" is removed.
Thread storage tests and registration
tests/internal/thread_storage.c, tests/internal/CMakeLists.txt
Adds TLS-backed tests for concurrent per-thread access and get-after-init behavior, registers them in TEST_LIST, and includes the new test source in the internal unit test list.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • fluent/fluent-bit#11090: Changes the same TLS initialization path and thread_storage behavior that these new tests exercise.

Suggested reviewers: edsiper, fujimotos

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is specific and matches the TLS detection fix in the changeset.
Linked Issues check ✅ Passed The PR replaces the broken C-TLS probe and adjusts initialization/tests, addressing the musl startup crash in #12065.
Out of Scope Changes check ✅ Passed The changes stay focused on TLS detection, thread storage initialization, and related tests with no clear unrelated additions.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cosmo0920-mitigate-c-tls-fallback-incorrectly-working

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/internal/thread_storage.c (1)

13-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding multi-threaded coverage.

This test validates the single-thread get/set/init sequence, but the actual fix addresses pthread_once-guarded concurrent key initialization across multiple threads. A test spawning several threads that call FLB_TLS_GET/FLB_TLS_SET concurrently would more directly cover the race this PR fixes.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/internal/thread_storage.c` around lines 13 - 30, The current
test_thread_storage_get_before_init only covers a single-thread get/set/init
flow and does not exercise the pthread_once-protected concurrent initialization
path. Add a new multi-threaded test around thread_storage_ctx that spawns
several threads and has them call FLB_TLS_GET and FLB_TLS_SET concurrently, then
assert the TLS key is initialized safely and the values remain consistent across
threads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/internal/thread_storage.c`:
- Around line 13-30: The current test_thread_storage_get_before_init only covers
a single-thread get/set/init flow and does not exercise the
pthread_once-protected concurrent initialization path. Add a new multi-threaded
test around thread_storage_ctx that spawns several threads and has them call
FLB_TLS_GET and FLB_TLS_SET concurrently, then assert the TLS key is initialized
safely and the values remain consistent across threads.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3052c79e-38ee-41bb-aef9-88e8cac11784

📥 Commits

Reviewing files that changed from the base of the PR and between 46e15a1 and 7bc011f.

📒 Files selected for processing (4)
  • CMakeLists.txt
  • include/fluent-bit/flb_thread_storage.h
  • tests/internal/CMakeLists.txt
  • tests/internal/thread_storage.c

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/internal/thread_storage.c`:
- Around line 34-41: The shared condition variable usage in the thread setup
flow can leave the test stuck because the same cond is used for both readiness
and start synchronization. Update the thread coordination in thread_storage.c,
specifically the logic around the worker-side ready increment and the main-side
wait loop, so that the ready notification wakes all waiters or the ready/start
phases use separate condition variables. Keep the existing symbols
thread->ready, thread->start, thread->cond, and the wait/signaling code in sync
so no worker can consume a wakeup intended for the main thread.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8bb20306-237c-43e0-a3b2-ebe93cf7cb93

📥 Commits

Reviewing files that changed from the base of the PR and between 7bc011f and 4142982.

📒 Files selected for processing (1)
  • tests/internal/thread_storage.c

Comment thread tests/internal/thread_storage.c
@cosmo0920 cosmo0920 force-pushed the cosmo0920-mitigate-c-tls-fallback-incorrectly-working branch from 4142982 to 71d955b Compare July 7, 2026 05:39
@cosmo0920 cosmo0920 changed the title thread_storage: mitigate c tls fallback incorrectly working thread_storage: Fix incorrectly to detect C TLS existence Jul 7, 2026
@patrick-stephens patrick-stephens added ok-package-test Run PR packaging tests and removed docs-required labels Jul 7, 2026
@edsiper

edsiper commented Jul 8, 2026

Copy link
Copy Markdown
Member

is there a way we can avoid calling pthread_once() ? since it will be called over and over, maybe fix the root cause where the initialization is missing ?

cosmo0920 added 2 commits July 9, 2026 12:09
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
@cosmo0920

cosmo0920 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

is there a way we can avoid calling pthread_once() ? since it will be called over and over, maybe fix the root cause where the initialization is missing ?

Oh! The main fix of this PR is incorrectly C TLS detection. So, I removed the pthread_once usages and tweak initialization order of worker context.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
tests/internal/thread_storage.c (1)

34-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Shared condition variable can still hang the test

The same cond is used for both the "ready" (worker→main) and "start" (main→workers) phases. At line 36, pthread_cond_signal wakes only one waiter. If a previously-created worker (waiting at line 39 on start == FALSE) consumes the signal instead of main (waiting at line 103 on ready < created), the signal is wasted. After the last worker signals, ready == created but main may still be asleep in pthread_cond_wait with no remaining signals to wake it — deadlock.

This was previously flagged and marked addressed, but the current rewritten file still uses pthread_cond_signal at line 36. Changing to pthread_cond_broadcast ensures main is always woken.

🔒 Proposed fix: broadcast instead of signal on worker ready
     pthread_mutex_lock(thread->lock);
     (*thread->ready)++;
-    pthread_cond_signal(thread->cond);
+    pthread_cond_broadcast(thread->cond);
#!/bin/bash
set -euo pipefail

# 1. Confirm the current worker-side signal is still pthread_cond_signal (not broadcast)
echo "=== Worker-side signal (expect pthread_cond_signal) ==="
sed -n '34,41p' tests/internal/thread_storage.c

# 2. Confirm main-side wait and broadcast
echo "=== Main-side wait/broadcast ==="
sed -n '101,107p' tests/internal/thread_storage.c

# 3. Verify the test file is registered in the build
echo "=== CMakeLists registration ==="
rg -n 'thread_storage' tests/internal/CMakeLists.txt
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/internal/thread_storage.c` around lines 34 - 41, The worker-to-main
ready notification in thread_storage.c still uses pthread_cond_signal, which can
wake the wrong waiter and leave main blocked in the ready wait. Update the
synchronization in the worker setup path around the pthread_cond_signal call in
the thread helper so it uses pthread_cond_broadcast instead, ensuring any main
thread waiting on the shared condition variable is always awakened while
preserving the existing ready/start handshake in thread->ready and
thread->start.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@tests/internal/thread_storage.c`:
- Around line 34-41: The worker-to-main ready notification in thread_storage.c
still uses pthread_cond_signal, which can wake the wrong waiter and leave main
blocked in the ready wait. Update the synchronization in the worker setup path
around the pthread_cond_signal call in the thread helper so it uses
pthread_cond_broadcast instead, ensuring any main thread waiting on the shared
condition variable is always awakened while preserving the existing ready/start
handshake in thread->ready and thread->start.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 08eddafa-fcf5-4b7e-8717-beeca469d504

📥 Commits

Reviewing files that changed from the base of the PR and between 71d955b and 645d6ed.

📒 Files selected for processing (4)
  • CMakeLists.txt
  • src/flb_config.c
  • tests/internal/CMakeLists.txt
  • tests/internal/thread_storage.c
✅ Files skipped from review due to trivial changes (1)
  • tests/internal/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
  • CMakeLists.txt

cosmo0920 added 2 commits July 9, 2026 12:34
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
@cosmo0920 cosmo0920 force-pushed the cosmo0920-mitigate-c-tls-fallback-incorrectly-working branch from 645d6ed to debb55e Compare July 9, 2026 03:35
@edsiper edsiper merged commit a41cdbb into master Jul 11, 2026
147 of 150 checks passed
@edsiper edsiper deleted the cosmo0920-mitigate-c-tls-fallback-incorrectly-working branch July 11, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-package-test Run PR packaging tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fluent-bit crashes with musl libc

3 participants